Skip to content

graphql: add field yParity#471

Merged
fjl merged 2 commits intoethereum:mainfrom
jsvisa:regen-graphql
Nov 8, 2023
Merged

graphql: add field yParity#471
fjl merged 2 commits intoethereum:mainfrom
jsvisa:regen-graphql

Conversation

@jsvisa
Copy link
Copy Markdown
Contributor

@jsvisa jsvisa commented Sep 21, 2023

No description provided.

Signed-off-by: jsvisa <delweng@gmail.com>
Copy link
Copy Markdown
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@s1na
Copy link
Copy Markdown
Contributor

s1na commented Nov 2, 2023

@shemnon Please take a look. This is consistent with the JSON-RPC. We have to return yParity for type-1 and type2 transactions instead of v. (More context: paradigmxyz/reth#3798)

@shemnon
Copy link
Copy Markdown
Contributor

shemnon commented Nov 2, 2023

Looks reasonable.

Why Long over BigInt? JSON-RPC encodes yParity as a uint not a uint64, and Long limits to 64. It's not an input parameter so queries passing in numbers shouldn't matter.

@s1na
Copy link
Copy Markdown
Contributor

s1na commented Nov 2, 2023

Given that r, s and v are also BigInt I tend to agree 👍

Comment thread graphql.json Outdated
Comment thread schema.graphqls Outdated
@s1na
Copy link
Copy Markdown
Contributor

s1na commented Nov 2, 2023

Come to think of it, v should become nullable. It will be null for type-1 and type-2 txes.

@shemnon
Copy link
Copy Markdown
Contributor

shemnon commented Nov 3, 2023

we also need to merge #468 which impacts the graphql apis.

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Nov 3, 2023

The value of yParity is always 0 or 1. So it makes no sense for it to be a BigInt. There are only two possible values in responses: 0x0 or 0x1. Not sure how this could be encoded into the schema.

@shemnon
Copy link
Copy Markdown
Contributor

shemnon commented Nov 3, 2023

The argument for BigInt is for consistent handling with mappings from the JSON-RPC api, because it is just uint there which maps to BigInt for r and s. Long is to much as well. What is really needed is a type that means "hex integer output" which is what BigInt fulfills.

@fjl fjl merged commit 0c18fb0 into ethereum:main Nov 8, 2023
@jsvisa jsvisa deleted the regen-graphql branch May 10, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants